-
Notifications
You must be signed in to change notification settings - Fork 4
Chrome Tabs Connection + Tab Grouping from Clusters #28
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
…s to gitignore for development.
…or this (tabs, history, activeTab). Added chrome.png. Added the connection to connection_manager. Added background script in background.ts to get all tabs in the browser. This connection also has 2 error handlers (not enough tabs and no tabs found) that more-readably show to the user when either is the case.
…b contents for better space creation. Added scripting permission to package.json to allow for tab reading to occur.
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
type fix Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
------------- New UI component with weird white + purple theme that's different from other UI components (maybe change in future? but not really a priority). Quality of Life: added 5-second timeout for getting data from any tab to stop that from blocking (happened once or twice for chrome:// tabs I think).
Summary of ChangesHello @Vodiboi, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the extension's capabilities by integrating direct interaction with Chrome browser tabs. It allows users to create a Mantis space from their open tabs, leveraging the content of each tab. Crucially, it introduces a feature to automatically organize these tabs into Chrome Tab Groups, using the clustering intelligence provided by Mantis. This streamlines tab management and provides a more structured browsing experience, especially for users with many open tabs. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request introduces a new "Chrome Tabs Connection" feature, which creates a Mantis space from open browser tabs and allows for organizing them into groups based on clusters derived from that space. The changes are substantial, particularly in the background script which now handles tab data fetching, content extraction, and WebSocket communication with the backend. My review highlights several critical issues that could prevent the feature from working correctly, including improper use of Chrome message listeners and a scope-related bug with injected scripts. I've also pointed out a type safety issue and a potential XSS vulnerability in the new UI code. The provided suggestions aim to resolve these bugs, enhance security, and improve the overall robustness of the implementation.
| chrome.runtime.onMessage.addListener((request, sender, sendResponse) => { | ||
| if (request.action === "fetchClusters") { | ||
| fetchClustersFromMantis(request.spaceId, new Map(request.tabsMap)) | ||
| .then((clusterGroups: [number, number[], string][]) => { // Add the string type for label | ||
| createTabGroupsFromClusters(clusterGroups) | ||
| .then(() => sendResponse({ success: true })) | ||
| .catch(error => sendResponse({ success: false, error: error.message })); | ||
| }) | ||
| .catch(error => sendResponse({ success: false, error: error.message })); | ||
|
|
||
| return true; // Keep message channel open for async response | ||
| } | ||
| }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You are adding multiple chrome.runtime.onMessage.addListener calls throughout this file. In a Chrome extension's background script, only one onMessage listener can be active at a time. Each new call to addListener overwrites the previous one, which will cause message handlers for actions like fetchClusters to be ignored. All message handling logic should be consolidated into a single onMessage listener, using a switch statement or an if/else if chain on request.action to delegate to the correct handler.
| const walker = document.createTreeWalker( | ||
| document.body || document.documentElement, | ||
| NodeFilter.SHOW_TEXT, | ||
| { acceptNode } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
acceptNode is called here, but it's defined in the background script's scope. This function getPageContent is executed in the tab's context where acceptNode will be undefined, causing a ReferenceError. The acceptNode function definition should be moved inside getPageContent to ensure it's in the correct scope when the script is executed.
| const result: [number, number[], string][] = Array.from(clusterGroups.entries()).map( | ||
| ([clusterId, tabIds]) => { | ||
| const label = clusterLabels.get(clusterId) || `Cluster ${clusterId}`; | ||
| return [clusterId as any, tabIds, label]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The clusterId is cast to any to satisfy the function's return type signature Promise<[number, number[], string][]>. However, clusterId is a string (likely a UUID), not a number. This bypasses type safety and can lead to bugs. The type signature should be updated to reflect the actual data type.
The return type of fetchClustersFromMantis (line 31), the type of clusterGroups (line 146), and the parameter for createTabGroupsFromClusters (line 6) should all be changed from [number, number[], string][] to [string, number[], string][]. Then, this as any cast can be safely removed.
| return [clusterId as any, tabIds, label]; | |
| return [clusterId, tabIds, label]; |
| modal.innerHTML = ` | ||
| <div style="text-align: center;"> | ||
| <div style="font-size: 48px; margin-bottom: 16px;">⚠️</div> | ||
| <h2 style="margin: 0 0 12px 0; font-size: 24px; color: #1a1a1a;">Failed to Organize</h2> | ||
| <p style="margin: 0 0 24px 0; color: #666; font-size: 15px;"> | ||
| ${error.message || 'An error occurred while organizing tabs.'} | ||
| </p> | ||
| <button id="close-error" style=" | ||
| background: #f0f0f0; | ||
| color: #666; | ||
| border: none; | ||
| padding: 12px 28px; | ||
| border-radius: 8px; | ||
| font-size: 15px; | ||
| font-weight: 600; | ||
| cursor: pointer; | ||
| ">Close</button> | ||
| </div> | ||
| `; | ||
| modal.querySelector('#close-error').addEventListener('click', () => { | ||
| overlay.style.opacity = '0'; | ||
| setTimeout(() => overlay.remove(), 300); | ||
| }); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You are using innerHTML to inject an error message into the DOM. The error.message can originate from the backend and could potentially contain malicious HTML, leading to a Cross-Site Scripting (XSS) vulnerability. You should use textContent to safely display the error message as plain text.
modal.innerHTML = `
<div style="text-align: center;">
<div style="font-size: 48px; margin-bottom: 16px;">⚠️</div>
<h2 style="margin: 0 0 12px 0; font-size: 24px; color: #1a1a1a;">Failed to Organize</h2>
<p id="mantis-organize-error-message" style="margin: 0 0 24px 0; color: #666; font-size: 15px;"></p>
<button id="close-error" style="
background: #f0f0f0;
color: #666;
border: none;
padding: 12px 28px;
border-radius: 8px;
font-size: 15px;
font-weight: 600;
cursor: pointer;
">Close</button>
</div>
`;
const p = modal.querySelector('#mantis-organize-error-message');
if (p) {
p.textContent = error.message || 'An error occurred while organizing tabs.';
}
modal.querySelector('#close-error').addEventListener('click', () => {
overlay.style.opacity = '0';
setTimeout(() => overlay.remove(), 300);
});
| const injectUI = async ( | ||
| space_id: string, | ||
| onMessage: onMessageType, | ||
| registerListeners: registerListenersType, | ||
| tabsMap?: Map<number, number> | ||
| ) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The injectUI function is defined with an optional tabsMap parameter, which is not part of the injectUIType interface. When this function is called from createSpace, the tabsMap argument is not provided, so it is undefined. The check if (!tabsMap) return null; on line 432 then causes the function to exit immediately, rendering this UI injection logic unused. Additionally, the showOrganizationPrompt function already seems to handle the UI for organizing tabs, making the purpose of this injectUI function unclear. The implementation needs to be revisited to either correctly wire it up or remove it if it's redundant.
|
|
||
|
|
||
| ws.addEventListener('message', (event) => { | ||
| const data = JSON.parse(event.data); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The WebSocket message data is parsed into an any type (JSON.parse(event.data)). This undermines the benefits of TypeScript. To improve type safety, code clarity, and maintainability, you should define interfaces for the expected WebSocket message structures and cast the parsed data to that type. For example:
interface Cluster {
id: string;
label?: string;
}
interface Point {
id: string;
cluster?: string;
metadata?: {
tab_id?: string;
};
}
interface WsMessage {
type: 'cluster' | 'points' | 'finished';
clusters?: Cluster[];
points?: Point[];
}
const data: WsMessage = JSON.parse(event.data);
What This PR Adds
This PR introduces the chrome tabs connection, which both makes a space from the mantis tabs and allows you to organize your tabs based on the clustrs from that space.
Features
Takes tabs and makes a space from them.
Organizes tabs into groups based on clusters from Mantis space
Technical Details
New connection: src/connections/chromeTabs/connection.tsx
Two new chrome message listeners in src/connections/background.ts (getting tab data and getting cluster data)
Testing
All existing tests pass
Extension builds successfully
Usage
All tabs have the mantis icon in the bottom right now. Select the tabs connection and run. Once its finished, can then organize the tabs in groups by clikcing the button in the new popup.